-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash in Beats on x86-64, minor other fixes #18131
Conversation
de368ea
to
accd9b1
Compare
@@ -429,6 +429,7 @@ namespace MIPSComp { | |||
skipStore = J_CC(CC_NE); | |||
|
|||
CompITypeMemWrite(op, 32, safeMemFuncs.writeU32); | |||
gpr.MapReg(rt, true, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this looks wrong. You can't map in a conditional, it could corrupt other regs from the perspective of the other condition. This needs to happen before the J_CC?
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the reg is only written to in both branches, I actually think it should be fine here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But hm, I'll clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because another reg might spill. If it only spills in one conditional, then that other reg is corrupted.
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A problem is that CompITypeMemWrite itself locks and unlocks regs, so I can't reliably do it that way, can I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think the actual bug is in CompITypeMemWrite, in the gpr.R(rt).IsImm() case, since normally we'd end up with rt being mapped coming out of it.
I'll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, not so much a bug as CompITypeMemWrite just not being designed to leave rt writable, fixable with a flag.
Fixes #18130 , the sc instruction failed to map the destination register.
Only use #18126 in soft-gpu mode (for now)
Add a couple of asserts.